Skip to content

test(integration): run arrow scenarios in CI instead of skipping#102

Merged
zfarrell merged 4 commits into
mainfrom
fix/integration-arrow-tests-100
Jun 6, 2026
Merged

test(integration): run arrow scenarios in CI instead of skipping#102
zfarrell merged 4 commits into
mainfrom
fix/integration-arrow-tests-100

Conversation

@zfarrell

@zfarrell zfarrell commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Closes #100.

Adds pyarrow to test-requirements.txt and replaces importorskip with direct imports so the two arrow scenarios actually run in CI instead of silently skipping. Also corrects managed_tables_lifecycle to real managed-catalog semantics: drops the refresh step (400 on a managed catalog) and polls get_table_profile for the async post-load sync.

claude[bot]
claude Bot previously approved these changes Jun 6, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified: pyarrow floor matches pyproject's arrow extra; direct imports correctly replace importorskip so CI fails loudly; managed-catalog polling loop handles 404 retry / non-404 re-raise / timeout cleanly. LGTM.

super nit (not blocking): this removed the only consumer of the refresh_api fixture, so refresh_api and the RefreshApi import in tests/integration/conftest.py are now dead and could be cleaned up.

claude[bot]
claude Bot previously approved these changes Jun 6, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified: pyarrow >= 14 floor matches the arrow extra in pyproject.toml, removed imports/fixtures are fully cleaned up, the ORDER BY makes the column/pagination asserts deterministic, and the managed-tables semantics changes are documented and still verify the load via the response. LGTM.

claude[bot]
claude Bot previously approved these changes Jun 6, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified: pyarrow floor matches the arrow extra in pyproject.toml, direct imports also clear the latent E402, removed pytest/refresh_api are genuinely unused, and the ORDER BY x change makes the row/pagination asserts deterministic. LGTM.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The drain-before-release fix is correct and well-tested, the ORDER BY makes the arrow assertions deterministic, and switching from importorskip to direct imports + pinning pyarrow in test-requirements.txt correctly forces these scenarios to run in CI.

@zfarrell zfarrell merged commit e01d7fc into main Jun 6, 2026
3 checks passed
@zfarrell zfarrell deleted the fix/integration-arrow-tests-100 branch June 6, 2026 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Integration tests silently skipped in CI (pyarrow not installed): managed_tables_lifecycle, results_arrow

1 participant